Zero-initialize pthread struct only in debug builds. NFC by kleisauke · Pull Request #26857 · emscripten-core/emscripten

kleisauke · 2026-05-05T08:58:26Z

Split out from #13007.

sbc100 · 2026-05-05T18:36:37Z


 int _emscripten_thread_is_valid(pthread_t thread) {
-  return thread->self == thread;
+  return thread->tid;


This seems more likely to return false positives if called on, e.g. an uninitialized region of memory?

This would align with musl, which uses the ->tid field to determine whether a thread is valid/available.

emscripten/system/lib/libc/musl/src/thread/pthread_create.c

Lines 110 to 117 in 4ceac4e

/* After the kernel thread exits, its tid may be reused. Clear it

* to prevent inadvertent use and inform functions that would use

* it that it's no longer available. At this point the killlock

* may be released, since functions that use it will consistently

* see the thread as having exited. Release it now so that no

* remaining locks (except thread list) are held if we end up

* resetting need_locks below. */

self->tid = 0;

https://github.com/search?q=repo%3Aemscripten-core%2Fmusl+-%3Etid+ESRCH&type=code

In any case, passing an uninitialized pthread_t to functions relying on _emscripten_thread_is_valid() is already undefined behavior.

Hmm, isn't the whole point of _emscripten_thread_is_valid to detect invalid/uninitialized pointers to pthreads?

I think it only makes sense to catch threads that have already terminated/joined. This also aligns with glibc (according to https://stackoverflow.com/a/78311551).

Commit 16c33d2 inlines _emscripten_thread_is_valid() to check for t->tid instead.

I see... but does isn't the memory pointed to by already terminated/joined memory that has been free'd? I.e. it contains basically undefined contents doesn;'t it?

Oh, I think I understand what you mean. In this code:

emscripten/system/lib/pthread/pthread_create.c

Lines 289 to 298 in 16c33d2

// Free the entire thread block (called map_base because

// musl normally allocates this using mmap). This region

// includes the pthread structure itself.

unsigned char* block = t->map_base;

dbg("_emscripten_thread_free_data thread=%p map_base=%p", t, block);

#ifndef NDEBUG

// To aid in debugging, set the entire region to zero.

memset(block, 0, sizeof(struct pthread));

#endif

emscripten_builtin_free(block);

->tid can end up in an undefined (dangling pointer) state because the entire pthread structure is free()'d. IIUC, this was already an issue before as well, since accessing ->self is also undefined behavior.

In any case, calling pthread_detach() or pthread_join() on a thread that has already been successfully joined is already UB, so it's unclear how much value there is in trying to make this logic "smarter".

Indeed, I think I just wanted to make it smart enough to satisfy the posixtest suite. In practice its undefined behaviour I think.

sbc100 · 2026-05-05T18:40:26Z

  unsigned char* block = t->map_base;
  dbg("_emscripten_thread_free_data thread=%p map_base=%p", t, block);
-  // To aid in debugging, set the entire region to zero.
-  memset(block, 0, sizeof(struct pthread));


Is it not useful for debugging to reset this memory to something ? Maybe 0xfe ? Maybe only in debug builds?

Commit e206e45 does this only for debug builds.

sbc100 · 2026-05-05T18:46:43Z


-  ret = pthread_join(self, NULL);
-  printf("pthread_join -> %s\n", strerror(ret));
-  assert(ret == EINVAL);


Do you know why we were testing this behaviour? Is this something that glibc does? Or maybe that the posixtests expect?

It was only needed for this test, according to this TODO comment:

emscripten/system/lib/libc/musl/src/thread/pthread_join.c

Lines 18 to 20 in 4ceac4e

// TODO: The detached check here is just to satisfy the

// `other.test_{proxy,main}_pthread_join_detach` tests.

if (t->detach_state != DT_DETACHED && __pthread_self() == t) return EDEADLK;

AFAIK, the posixtests didn't require this. I'm not entirely sure why we were testing this, I'll investigate.

Reverted this with commit 44d0df9 as I'm not sure what the reason for this is.

sbc100 · 2026-05-07T05:46:35Z

-	// `other.test_{proxy,main}_pthread_join_detach` tests.
-	if (t->detach_state != DT_DETACHED && __pthread_self() == t) return EDEADLK;
+	// XXX Emscripten: Add check for invalid (already joined) thread.
+	if (!t->tid) return ESRCH;


I kind of like the explicitness of having a _emscripten_thread_is_valid function.

Re-added with commit b19bce9.

sbc100 · 2026-05-07T05:48:27Z

+  // The pthread struct has a field that points to itself.
  new->self = new;
+
+  // Thread ID, this becomes zero when the thread is no longer available.


.. is no longer valid? Or .. is destroyed?

This comment was based on:

emscripten/system/lib/libc/musl/src/thread/pthread_create.c

Lines 110 to 117 in d137bef

/* After the kernel thread exits, its tid may be reused. Clear it

* to prevent inadvertent use and inform functions that would use

* it that it's no longer available. At this point the killlock

* may be released, since functions that use it will consistently

* see the thread as having exited. Release it now so that no

* remaining locks (except thread list) are held if we end up

* resetting need_locks below. */

self->tid = 0;

sbc100 · 2026-05-07T05:49:02Z

 #endif
+  // The tid may be reused. Clear it to prevent inadvertent use
+  // and inform functions that would use it that it's no longer
+  // available.


/available/valid/?

Same as above, this comment was synced with musl.

sbc100 · 2026-05-07T05:50:57Z

  dbg("_emscripten_thread_free_data thread=%p map_base=%p", t, block);
+#ifndef NDEBUG
  // To aid in debugging, set the entire region to zero.
  memset(block, 0, sizeof(struct pthread));


Should we actually use 0xdd (or some other recognizable value) rather than zero?

I'm not sure, but I think that would break the ->tid check in _emscripten_thread_is_valid().

sbc100 · 2026-05-07T05:53:44Z

+	if (!t->tid) return ESRCH;
+
+	// Note that we don't use pthread_join here, to avoid
+	// returning EDEADLK when attempting to detach itself.


If we had a self-check here could we share more of the code with musl?

Reverted this with commit b19bce9.

sbc100 · 2026-05-09T20:23:18Z

-	if (!_emscripten_thread_is_valid(t))
-		return ESRCH;
+	if (!_emscripten_thread_is_valid(t)) return ESRCH;
 #endif


Seems like you may as well just revert this file now? Unless you think the formatting is important here?

Sure, reverted with 982b8d2.

sbc100 · 2026-05-09T20:24:26Z


 int _emscripten_thread_is_valid(pthread_t thread) {
-  return thread->self == thread;
+  return thread->tid;


I still don't understand why would want to change this. The old code will catch more cases of invalid thread data won't it?

Reverted in commit ab9e35e. The previous discussion at #26857 (comment) is relevant here. I was trying to keep this in-sync with musl (+ glibc which does the same thing). Note that the posixtest suite still passes with this change.

sbc100 · 2026-05-09T20:25:08Z

+  // The tid may be reused. Clear it to prevent inadvertent use
+  // and inform functions that would use it that it's no longer
+  // available.
+  t->tid = 0;


t->self = NULL too and revert _emscripten_thread_is_valid?

Done in commit ab9e35e.

sbc100 · 2026-05-09T20:28:21Z


-  if (!--libc.threads_minus_1) libc.need_locks = 0;
+  __do_orphaned_stdio_locks();
+  __dl_thread_cleanup();


It seems like added these two function, and moving the emscripten_is_main_runtime_thread up are the two substantive remaining change.

Can you explain why each of those is good thing?

Reverted in commit 586b49f, this will be split into its own PR later. I was trying to keep it in-sync with musl.

... split to PR #26910.

Split out from #26857.

This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (2) test expectation files were updated by running the tests with `--rebaseline`: ``` codesize/test_codesize_minimal_pthreads.json: 26419 => 26409 [-10 bytes / -0.04%] codesize/test_codesize_minimal_pthreads_memgrowth.json: 26829 => 26819 [-10 bytes / -0.04%] Average change: -0.04% (-0.04% - -0.04%) ```

sbc100 reviewed May 5, 2026

View reviewed changes

kleisauke changed the title ~~Simplify thread clean / exit / terminate logic. NFC~~ Simplify thread clean / exit / terminate logic May 6, 2026

sbc100 reviewed May 7, 2026

View reviewed changes

kleisauke mentioned this pull request May 9, 2026

Remove unused field from pthread struct. NFC #26900

Merged

sbc100 reviewed May 9, 2026

View reviewed changes

kleisauke force-pushed the simplify-thread-exit branch 2 times, most recently from 42e8129 to 5ab021a Compare May 10, 2026 08:42

kleisauke changed the title ~~Simplify thread clean / exit / terminate logic~~ Zero-initialize the pthread struct only in debug builds. NFC May 10, 2026

kleisauke mentioned this pull request May 10, 2026

Sync _emscripten_thread_exit() with musl's pthread_exit #26910

Open

kleisauke force-pushed the simplify-thread-exit branch from 5ab021a to 0a20f22 Compare May 10, 2026 09:14

sbc100 pushed a commit that referenced this pull request May 10, 2026

Remove unused field from pthread struct. NFC (#26900)

00f352d

Split out from #26857.

sbc100 approved these changes May 10, 2026

View reviewed changes

kleisauke added 2 commits May 10, 2026 19:35

Zero-initialize pthread struct only in debug builds. NFC

8623e10

kleisauke changed the title ~~Zero-initialize the pthread struct only in debug builds. NFC~~ Zero-initialize pthread struct only in debug builds. NFC May 10, 2026

kleisauke force-pushed the simplify-thread-exit branch from 0a20f22 to 879bd13 Compare May 10, 2026 17:38

sbc100 merged commit d014c32 into emscripten-core:main May 10, 2026
30 checks passed

kleisauke deleted the simplify-thread-exit branch May 11, 2026 08:08

	/* After the kernel thread exits, its tid may be reused. Clear it
	* to prevent inadvertent use and inform functions that would use
	* it that it's no longer available. At this point the killlock
	* may be released, since functions that use it will consistently
	* see the thread as having exited. Release it now so that no
	* remaining locks (except thread list) are held if we end up
	* resetting need_locks below. */
	self->tid = 0;

	// Free the entire thread block (called map_base because
	// musl normally allocates this using mmap). This region
	// includes the pthread structure itself.
	unsigned char* block = t->map_base;
	dbg("_emscripten_thread_free_data thread=%p map_base=%p", t, block);
	#ifndef NDEBUG
	// To aid in debugging, set the entire region to zero.
	memset(block, 0, sizeof(struct pthread));
	#endif
	emscripten_builtin_free(block);

	// TODO: The detached check here is just to satisfy the
	// `other.test_{proxy,main}_pthread_join_detach` tests.
	if (t->detach_state != DT_DETACHED && __pthread_self() == t) return EDEADLK;

Conversation

kleisauke commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kleisauke May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kleisauke commented May 5, 2026 •

edited

Loading

kleisauke May 6, 2026 •

edited

Loading